ci: add newdeps job testing newer versions of cmake and capnproto #212
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. |
| if ! cmake "$src_dir" "${cmake_args[@]}"; then | ||
| # If cmake failed, try it again with debug options. | ||
| # Could add --trace / --trace-expand here too but they are very verbose. | ||
| cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG) |
There was a problem hiding this comment.
While this project maintains support for CMake versions as old as 3.12, I believe this support should also extend consistently to the CI infrastructure, particularly given the "olddeps" job, which runs CMake 3.12.4. Therefore, features incompatible with CMake 3.12, such as --debug-find and --log-level, should be avoided. For example, the "olddeps" CI job may result in:
+ cmake /home/runner/work/libmultiprocess/libmultiprocess --debug-find --debug-output --debug-trycompile --log-level=DEBUG
CMake Error: The source directory "/home/runner/work/libmultiprocess/libmultiprocess/build-olddeps/--log-level=DEBUG" does not exist.
Alternatively, the minimum supported CMake version could be bumped.
There was a problem hiding this comment.
re: #212 (comment)
For example, the "olddeps" CI job may result in
If the problem you would like me to solve is that extra output potentially appearing in the olddeps job, it is trivial to skip the debug step there with:
-if ! cmake "$src_dir" "${cmake_args[@]}"; then
+if ! cmake "$src_dir" "${cmake_args[@]}" && ver_ge "$cmake_ver" "3.17"; thenBut I don't see a problem with this extra output and I don't know if this the issue you actually care about. If you are arguing that systems using cmake 3.16 should not be supported because it does not support the --debug-find option, than that sounds like something to mention in #175 more than here.
| # Could add --trace / --trace-expand here too but they are very verbose. | ||
| cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG) | ||
| cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?" | ||
| cat CMakeFiles/CMakeConfigureLog.yaml || true |
There was a problem hiding this comment.
Moving discussion from #209 (comment).
re: #209 (comment)
6ba1050
This log file name has been used only since CMake 3.26.Yes that's part of the reason for adding
|| truehere. I think this code could potentially print log files used by older versions of cmake too, and originally this change tried that, but since all CI jobs except one are using new enough versions of cmake it didn't seem worth complexity.
One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?
Would happily accept a PR to patch to improve this though.
This would make sense if it were the only compatibility issue in this PR.
There was a problem hiding this comment.
re: #212 (comment)
One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?
It would be helpful to know what complexity you are referring to specifically here. The find_packge(Threads REQUIRED) issue would have been impossible to debug without the line:
cat CMakeFiles/CMakeConfigureLog.yaml || true
and I do not feel like it adds much complexity. This CI script in general seems short and simple to me. I don't know what problem you have with the other debug prints either. In general, I feel like it would be a lot easier to respond to your comments if they could make it clear:
- What observable problem you think this change causes, or could potentially cause.
- What change you would suggest to fix the problem.
There was a problem hiding this comment.
Appreciate your review, even though I am having trouble understanding what your practical concerns are and what specific changes I could make that would address them.
Hopefully we both accept the premise that it is good to have a new CI job testing newer versions of cmake and capnproto, and it is good to show more debug output when cmake configuration fails.
| # Could add --trace / --trace-expand here too but they are very verbose. | ||
| cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG) | ||
| cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?" | ||
| cat CMakeFiles/CMakeConfigureLog.yaml || true |
There was a problem hiding this comment.
re: #212 (comment)
One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?
It would be helpful to know what complexity you are referring to specifically here. The find_packge(Threads REQUIRED) issue would have been impossible to debug without the line:
cat CMakeFiles/CMakeConfigureLog.yaml || true
and I do not feel like it adds much complexity. This CI script in general seems short and simple to me. I don't know what problem you have with the other debug prints either. In general, I feel like it would be a lot easier to respond to your comments if they could make it clear:
- What observable problem you think this change causes, or could potentially cause.
- What change you would suggest to fix the problem.
| if ! cmake "$src_dir" "${cmake_args[@]}"; then | ||
| # If cmake failed, try it again with debug options. | ||
| # Could add --trace / --trace-expand here too but they are very verbose. | ||
| cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG) |
There was a problem hiding this comment.
re: #212 (comment)
For example, the "olddeps" CI job may result in
If the problem you would like me to solve is that extra output potentially appearing in the olddeps job, it is trivial to skip the debug step there with:
-if ! cmake "$src_dir" "${cmake_args[@]}"; then
+if ! cmake "$src_dir" "${cmake_args[@]}" && ver_ge "$cmake_ver" "3.17"; thenBut I don't see a problem with this extra output and I don't know if this the issue you actually care about. If you are arguing that systems using cmake 3.16 should not be supported because it does not support the --debug-find option, than that sounds like something to mention in #175 more than here.
Add CI job testing latest released version of cmake, and latest unreleased version of cap'n proto to uncover any problems there may be with new releases. Use git version of cap'n proto because we've submitted patches to that project to fix issues in the past, so it would be good to know about any new issues as soon as possible.
Also try to improve CI output since current output has not been very good for debugging, by adding git log calls, cmake debug options, and a dump of the cmake configure log on failure.